Add opt-in owner permission normalization to @tryghost/zip extract#761
Merged
Conversation
- Adds an `ensureOwnerPermissions` option (default `false`) to `extract()` - When enabled, normalizes extracted entry permissions so the owner can always read/move/remove the result: directories gain at least owner rwx and files gain at least owner rw, while existing execute/group/world bits are preserved - Fixes archives containing read-only directories (e.g. `dr-xr-xr-x`/`0555`) that otherwise fail to extract because nested files cannot be written into them - Normalization mutates only the in-memory entry handed to extract-zip; the source zip is never rewritten or chmodded, and it runs after the existing size-limit, symlink and filename checks - Defaults are unchanged for existing callers Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
WalkthroughThe PR adds a shared entry-mode helper and a new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #761 +/- ##
==========================================
+ Coverage 98.04% 98.42% +0.38%
==========================================
Files 84 5 -79
Lines 2759 127 -2632
Branches 506 26 -480
==========================================
- Hits 2705 125 -2580
+ Misses 12 1 -11
+ Partials 42 1 -41 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eaef85d500
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ions Previously, entries with no POSIX mode bits early-returned from ensureOwnerPermissions and were handed to extract-zip as mode 0. When the caller combined `ensureOwnerPermissions: true` with a `defaultDirMode`/ `defaultFileMode` that omits owner bits (e.g. 0o555/0o444), extract-zip synthesized that owner-less default, so a no-POSIX-mode directory/file could still be created without owner write — violating the option's guarantee. Now zero-mode entries resolve the same default extract-zip would apply (mirroring its getExtractedMode fallback) and then have the owner bits OR-ed in, so the owner can always read/move/traverse the extracted tree regardless of the supplied defaults. The default case (no custom defaults) is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The JSDoc for `defaultDirMode`, `defaultFileMode`, `onEntry`, and the `limits`
sub-fields omitted the `[ ]` optional markers, so the types generated from the
JSDoc treated them as required. TypeScript consumers (e.g. gscan) could not call
`extract(zip, dest, {ensureOwnerPermissions: true})` without also supplying those
params. They all have defaults or are "if present", so they are now correctly
marked optional.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
mike182uk
approved these changes
Jun 25, 2026
…sting Moves the permission-normalization logic out of extract.js into: - lib/ensure-owner-permissions.js - the pure `(entry, opts)` normalizer - lib/entry-mode.js - shared getEntryMode() + POSIX stat constants This lets the permission tests assert on the function directly with synthetic entries instead of patching Module._load to mock extract-zip just to drive the onEntry callback. The four affected tests are now plain, synchronous unit tests with no module mocking, and the Windows directory-attribute path (previously impractical to reach through the extract() pipeline) is now covered too. Behavior is unchanged; the real-archiver extraction tests still cover the end-to-end path where extract-zip honors the rewritten attributes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/zip/README.md`:
- Around line 37-39: The fenced code example in the README is missing a language
identifier, which triggers markdownlint MD040. Update the example fence in the
zip README to use a JavaScript language tag, keeping the existing extract usage
snippet unchanged; this is the block containing zip.extract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a88824d3-485b-41a4-aa83-cd6429a7f2ca
📒 Files selected for processing (5)
packages/zip/README.mdpackages/zip/lib/ensure-owner-permissions.jspackages/zip/lib/entry-mode.jspackages/zip/lib/extract.jspackages/zip/test/zip.test.js
This was referenced Jun 25, 2026
sagzy
added a commit
to TryGhost/gscan
that referenced
this pull request
Jun 25, 2026
ref TryGhost/framework#761 ref https://linear.app/ghost/issue/ONC-1838/timeout-on-theme-upload Some theme zip files have read-only folder permissions. When an user uploads one of these themes, GScan needs to unzip the archive and create files inside those folders. If the folders are not writable by the owner, extraction can get stuck and the theme upload appears to freeze in the UI without a useful error message. With this change, GScan opts into the new @tryghost/zip@3.5.0 owner-permission normalization while extracting theme uploads. Directories are normalized so the owner can read, write, and traverse them, and files are normalized so the owner can read and write them. The original zip is not modified; only the temporary extracted copy is made usable for validation and installation. The result is that themes with incorrect folder permissions can still be uploaded successfully instead of leaving the user stuck on a frozen upload.
sagzy
added a commit
to TryGhost/Ghost
that referenced
this pull request
Jun 25, 2026
ref https://linear.app/ghost/issue/ONC-1838 ref TryGhost/framework#761 ref TryGhost/gscan#853 Theme uploads could hang without any visible error when a zip contained folders without sufficient write permissions. GScan needs to write into those folders while extracting and validating the theme. Updated `gscan` to include the permission handling fix so missing permissions are restored during upload and the theme can be installed successfully instead of leaving the UI frozen.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref https://linear.app/ghost/issue/ONC-1838
Summary
Adds an opt-in
ensureOwnerPermissionsoption to@tryghost/zip'sextract()that normalizes extracted entry permissions beforeextract-zipwrites them. This fixes archives that contain read-only directories (e.g.dr-xr-xr-x/0555) without changing default behavior for existing callers.Why
Some user-supplied archives (e.g. theme zips) store directories as read-only (
0555). Whenextract-ziprecreates such a directory first and then tries to write a nested file into it, extraction fails withEACCES. Even when a read-only directory has no children, it lands on disk without owner write, so the caller can't subsequently move or remove the extracted tree.A recursive post-extract
chmodis not a robust primary fix, because the bad directory modes break extraction before any post-processing could run. Normalizing per-entry, just before the write, fixes the failure at the source.What changed
packages/zip/lib/extract.js:options.ensureOwnerPermissions?: boolean(defaultfalse).true, each entry's mode is normalized in the wrappedonEntrycallback:rwx(0o700).rw(0o600).extract-zipexactly (POSIX dir bit, trailing slash, Windows directory attribute); inferred directories without POSIX type bits get the directory type bit set.extract-zip's own default modes still apply.extract-zipis mutated — the original zip is never rewritten or chmodded.onEntry, so all existing safety checks still apply. No async work is added insideonEntry(extract-zip does not await it).throwOnSymlinksto share the newgetEntryModehelper and mode constants (behavior unchanged).packages/zip/README.md: documented the new option and its intended use (trusted temporary extraction of user-supplied archives where the caller must be able to read, move, and remove the extracted tree).Tests
Added Vitest coverage that builds zips programmatically with
archiver(no dependency on the systemzip):ensureOwnerPermissions: truesuccessfully extracts a zip with a0555directory, a nested file, a read-only file (owner write added), and an already-writable file (left untouched).SYMLINK_NOT_ALLOWEDwith the option enabled.pnpm --filter @tryghost/zip test→ 21 passing; lint andoxfmt --checkclean.extract.jsis at 100% line / 98% branch coverage.Notes for reviewers
ensureOwnerPermissionsin a follow-up when validating uploaded theme zips.extract-zipmasks the on-disk mode to0o777, so no setuid/setgid/sticky escalation is possible.